fix: preserve decision/loop captions across nested control flow#263
Conversation
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove the PR. The fix is correct, well-tested, and ready for merge. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
# Conflicts: # mdl/executor/cmd_microflows_builder_control.go
Code Review — fix: preserve decision/loop captions across nested control flowOverviewClean fix for shared-state contamination in the flow builder. The root cause is clear from reading
The fix is correct: snapshot + clear before recursion, re-apply after the activity is created. Correctness
No double-apply —
Tests
GapsLoop caption fix is untested —
No MDL bug-test script — CLAUDE.md checklist requires MinorThe test comment "splits are appended in creation order: outer first" is an implementation assumption rather than a contract. It's stable given the current builder logic, but worth flagging as a test fragility if the builder is ever reordered. VerdictLGTM with minor gaps. The core fix is correct and well-tested for the primary case. The loop/InheritanceSplit coverage gaps and missing MDL script are worth a follow-up but are not blockers given the identical pattern is tested on the IF side. |
When an IF, LOOP, or WHILE statement carried an @caption or @annotation and contained other annotated statements in its body, the inner statement's annotations would overwrite fb.pendingAnnotations (shared builder state) before the outer loop in buildFlowGraph attached them to the right object. The outer split/loop then ended up with the wrong caption — or with the inner statement's caption applied to it. The fix snapshots & clears pendingAnnotations at the top of each compound statement handler, re-applies them to the split/loop/while activity right after it's created, and (for IF) moves applyAnnotations above the branch recursion so the split is decorated before the bodies are walked. Also extends applyAnnotations to recognise ExclusiveSplit and InheritanceSplit — on main these were only handled for ActionActivity, so @caption on a split was silently dropped even without nesting. Verified end-to-end against Apps.GetOrCreateMendixVersionFromString: describe -> exec -> describe now produces byte-identical @caption, @annotation, and @position output. Regression tests cover: - nested IFs with explicit @caption on each level - single IF @caption baseline - @annotation attachment targeting the correct split when nested Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mergeStatementAnnotations: add WhileStmt case (was falling through to default: nil, so @caption on a WHILE was silently dropped). - applyAnnotations: add LoopedActivity case — LOOP and WHILE activities can now carry the captions they already parse. - Add TestLoopCaptionPreserved, TestWhileLoopCaptionPreserved, and TestInheritanceSplitCaptionApplied to cover the gaps ako flagged (loop and InheritanceSplit caption paths were previously untested). - Add `mdl-examples/bug-tests/263-nested-caption-preservation.mdl` reproducer per CLAUDE.md checklist.
hjotha
force-pushed
the
submit/microflow-nested-caption-preservation
branch
from
48881e7 to
94bded8
Compare
|
Thanks @ako — addressed the LGTM gaps in 94bded8:
While wiring the loop caption test I also caught two related gaps the original fix didn't close:
Both gaps are covered by the new tests. Re: the minor note about "splits appended in creation order: outer first" — fair flag. Left the comment as-is for now since reordering the builder would be a separate refactor and the assumption is documented in the test itself rather than hidden in the production code. |
AI Code ReviewWhat Looks Good
RecommendationApprove the PR. The fix is correct, well-tested, and maintains code quality standards. It resolves the reported issue without introducing regressions or violating project conventions. Ready for merging. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
3 participants
Summary
@caption '...'annotations on nested decisions (if) and loops were being dropped when the describer emitted MDL. The annotation state on the flow builder was overwritten by each child statement as it was processed, so when the outer loop inbuildFlowGraphconsumed the pending annotations for a parent, it picked up the wrong ones.Fix snapshots each IF's / loop's own pending annotations into local scope before recursing into children, then re-applies them to the outer split/loop after the body is processed. Captions on nested control flow now round-trip.
Part of umbrella #257.
Test plan
@captionon nestedifinsideloopinsideif.go build ./... && go test ./...pass.Stack
This PR is the base of a 3-PR stack. The other two PRs build on it:
The stacking is needed because all three touch shared structs in the flow-graph builder; duplicating fields would produce nasty conflicts.